-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New unit for NetHTTP #33
Conversation
src/Notify.Api.Factory.pas
Outdated
|
||
function TNotifyApiFactory.Api: INotifyApi; | ||
begin | ||
{$IFDEF INDY} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs documenting in the README.md. It's effectively a part of your public interface "how to switch between Indy/NetHTTP".
Also, consider renaming this compiler define to NTFY_INDY
. Consider, with such a generic define based on the name of a very popular network component set, what would happen if your library was included in other large projects that may have libraries with the same defines for other purposes? You may even consider having a more descriptive name NTFY_DO_A_THING_WITH_INDY
so it becomes self-documenting when people include it in their projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking out for improvement. I'll make the necessary changes. I think NTFY_INDY
is more descriptive name as you mentioned as it avoids clashing with other name that a large library might have. I'll also provide documentation of that in the README.md file. I'll update the files with the above review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhatherall, your review has been submitted. Thanks for your contribution.
- Updated conditional defines for switching between Indy and NetHTTP - Updated readme documentation - FreeAndNil removed from Api classes.
♻ Refactoring
|
♻ NetHTTP Compatibility issues.
function TNotifyApiNetHTTP.ClearHeaders: INotifyApi;
begin
Result := Self;
FNetHTTPRequest.CustHeaders.Clear; //does not exists in Delphi < 10.4 Sydney
end; procedure TSSEThread.AbortStream;
begin
FNetHTTPResquest.Cancel; //Does not exists in Delphi < 10.4 Sydney
end; |
Unfortunately, I could not find a solution to cancel the long polling connection with NetHTTP for mantain retro-compatibility. NetHTTP class was simply incomplete from product version 26 to 27 and that was a shock to me. If you are using a product version lower or equal than 26 (10.3) you'll not be able to unsubscribe from a topic unless you close your application or service.
From product version 26 to 27, Embarcadero completed the NetHTTP class to perform an abort operation on current requests in transaction. Although many years has passed since the first release of NetHTTP, this class was incomplete and lacking these basic methods even up until the release 26 - something that shocked me when I realized. This is a huge drawback if you are using Delphi 10.3 and lower. It seems they didn't have in mind that a client could possibly perform a long polling operation. Because of that, if we are using a product version lower or equal to 26 with NetHTTP, the library will be unable to unsubscribe from a topic unless we close and restart the application or service. Yes, huge drawback for retro-compatibility. Otherwise, everything remains as it before. From Delphi 10.4+, nothing has been changed except this new NetHTTP class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready. I'm just working on a small example for Android subscriber.
Sorry guys, is being a long time but I finally got the opportunity to continue the project. This is not an example of how to follow a milestone. Anyhow, it seems that NetHTTP is now fully operational and maintaining long polling operations without issues. As from my investigation the last year, I had several problems to perform this and even the "Cancel Request" method wasn't behaving as should. I'm not sure if Embarcadero did changes to NetHTTP this last version but seems very stable now. I was able to maintain more than 10h of long polling connection without timeout or hang. When I first saw that working was like melody to my ears and I could finally perform the tests I needed to Android. I prepared an example under Long story short, I'm happy to merge the PR with main and delete this branch once for all. |
NTFY_HTTP_INDY
.Without this compiler directive, the library will utilize NetHTTP by default. The developer can alter accessing
Project > Options > Building > Delphi Compiler > Conditional Defines
and addingNTFY_HTTP_INDY
on the conditional defines list.Sample
directory was renamed tosamples
, so that the dependency manager ignores this folder when installing, avoiding the units inside this directory to be added on the project's search path.A new example was included demonstrating how utilizing this library on windows services.
Two warnings were also removed at Notify.Api.Indy unit. They were caused by implicit cast operation when a string variable was assigned to a UTF8String variable.
TNotifyResponseData was renamed to TNotifyResponseDTO.
In case the developer opts to utilize Indy components, the openssl libraries will be loaded according to the operational sistem of the client. It has not yet been tested in some operational systems.